Add ACNP appliedto NodePort svc support #3997

GraysonWu · 2022-07-13T01:24:33Z

With this PR users could use ACNP to control the external access of a NodePort Service.

Add service field in appliedTo of ACNP.
Add Service in AppliedToGroup and appliedToGroup with Service will span to all Nodes.
Use groupIDs of a Service to do destination matching.
Process ipBlock to ct_nw_src to do source matching.

Some API-wise restrictions:

Only an ingress rule or a policy only has ingress rules can use service in its appliedTo.
service field can't be used with other selectors in appliedTo.
If a policy or an ingress rule is applied to Services, that only ipBlock can be used to select workloads.
A policy or an ingress rule can't be applied to Services and other workloads at the same time.
appliedTo like below is invalid:

appliedTo:
      - service:
           name: svc
           namespace: default
      - podSelector:
           matchLabels: 
               app: client

The reason for 3 & 4 is that we use service field to decide if a policy or ingress rule is a NodePort Service policy/rule. If it is a NodePort Service policy/rule, we can only use ct_nw_src, transformed from ipBlock, to do the source matching. The source matching logic is different from normal policy/rule and NodePort Service policy/rule.

Signed-off-by: wgrayson [email protected]

codecov · 2022-07-13T01:35:20Z

Codecov Report

Merging #3997 (c4034ad) into main (684dca3) will increase coverage by 2.63%.
The diff coverage is 62.83%.

@@            Coverage Diff             @@
##             main    #3997      +/-   ##
==========================================
+ Coverage   64.45%   67.08%   +2.63%     
==========================================
  Files         294      297       +3     
  Lines       43726    45846    +2120     
==========================================
+ Hits        28184    30758    +2574     
+ Misses      13252    12704     -548     
- Partials     2290     2384      +94

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`40.54% <38.23%> (?)`
integration-tests	`35.84% <4.49%> (?)`		Carriedforward from bbf780c
kind-e2e-tests	`50.50% <51.33%> (-0.14%)`	⬇️
unit-tests	`44.44% <56.13%> (+0.23%)`	⬆️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/controller/networkpolicy/reject.go	`83.51% <0.00%> (-2.36%)`	⬇️
pkg/apis/controlplane/types.go	`0.00% <ø> (ø)`
pkg/controller/types/networkpolicy.go	`100.00% <ø> (ø)`
pkg/apis/controlplane/sets.go	`22.22% <14.28%> (-6.90%)`	⬇️
pkg/agent/openflow/network_policy.go	`79.15% <26.53%> (-0.38%)`	⬇️
...g/controller/networkpolicy/clusternetworkpolicy.go	`68.18% <38.88%> (-1.04%)`	⬇️
pkg/agent/openflow/pipeline.go	`82.17% <40.74%> (+8.86%)`	⬆️
...ntroller/networkpolicy/networkpolicy_controller.go	`80.14% <46.15%> (-2.19%)`	⬇️
pkg/apis/controlplane/v1beta2/sets.go	`46.21% <71.42%> (-8.22%)`	⬇️
pkg/agent/controller/networkpolicy/cache.go	`89.02% <77.77%> (+0.23%)`	⬆️
... and 83 more

pkg/controller/types/networkpolicy.go

pkg/apis/controlplane/types.go

pkg/apis/controlplane/v1beta2/types.go

pkg/apis/crd/v1alpha1/types.go

GraysonWu · 2022-07-15T00:49:53Z

Looks like there is a race condition that will cause deadlock. Working on it. Will address @jianjuns 's comments in the same commit.

lgtm-com · 2022-07-19T02:36:11Z

This pull request introduces 1 alert when merging 0dacb3f into 231b09d - view on LGTM.com

new alerts:

1 for Off-by-one comparison against length

lgtm-com · 2022-07-19T03:49:39Z

This pull request introduces 1 alert when merging 0cc328f into 231b09d - view on LGTM.com

new alerts:

1 for Off-by-one comparison against length

docs/antrea-network-policy.md

pkg/controller/networkpolicy/clusternetworkpolicy.go

tnqn · 2022-07-21T17:19:32Z

If a policy or an ingress rule is applied to Services, that only ipBlock can be used to select workloads.

A policy or an ingress rule can't be applied to Services and other workloads at the same time.
appliedTo like below is invalid:

The reason for 3 & 4 is that we use service field to decide if a policy or ingress rule is a NodePort Service policy/rule. If it is a NodePort Service policy/rule, we can only use ct_nw_src, transformed from ipBlock, to do the source matching.

I'm not against to support IPBlocks as peer only given that we apply this to NodePort Service only which is typically accessed by external network. However, this explanation seems to say it's because of implementation restriction. I don't get why we we not use ct_nw_src for Pod IPs of a normal AddressGroup.

GraysonWu · 2022-07-21T22:14:56Z

If a policy or an ingress rule is applied to Services, that only ipBlock can be used to select workloads.

A policy or an ingress rule can't be applied to Services and other workloads at the same time.
appliedTo like below is invalid:

The reason for 3 & 4 is that we use service field to decide if a policy or ingress rule is a NodePort Service policy/rule. If it is a NodePort Service policy/rule, we can only use ct_nw_src, transformed from ipBlock, to do the source matching.

I'm not against to support IPBlocks as peer only given that we apply this to NodePort Service only which is typically accessed by external network. However, this explanation seems to say it's because of implementation restriction. I don't get why we we not use ct_nw_src for Pod IPs of a normal AddressGroup.

There are two reasons:

If a client Pod on Node A tries to access Node B NodePort Service when the packet arrives Node B, it has already been SNAT'd to Node A IP. The client Pod IP can't be matched by nw_src or ct_nw_src on Node B.
If the client Pod and the endpoint of the Service are the same Node when the client Pod initiates traffic to its local NodePort Service, the packet won't be SNAT'd. We can't use ct_nw_src to match it.

Let me know if I misunderstood something.

Other than the implementation reason, I totally agree with you that a NodePort Service is typically accessed by an external network. In-cluster Pods will usually use Service ClusterIP. IMO, supporting podSelector for a NodePort Service doesn't make too much sense.

tnqn · 2022-07-22T12:05:05Z

There are two reasons:

If a client Pod on Node A tries to access Node B NodePort Service when the packet arrives Node B, it has already been SNAT'd to Node A IP. The client Pod IP can't be matched by nw_src or ct_nw_src on Node B.

If the client Pod and the endpoint of the Service are the same Node when the client Pod initiates traffic to its local NodePort Service, the packet won't be SNAT'd. We can't use ct_nw_src to match it.

1 makes sense. 2 is not true. ct_nw_src will just be original src IP if it's not SNAT'd.

pkg/agent/controller/networkpolicy/cache.go

pkg/agent/controller/networkpolicy/reject.go

pkg/controller/networkpolicy/networkpolicy_controller.go

GraysonWu · 2022-07-31T02:05:36Z

/test-integration

GraysonWu · 2022-07-31T02:06:26Z

/test-all

tnqn

LGTM overall

pkg/agent/controller/networkpolicy/reconciler.go

pkg/agent/controller/networkpolicy/reject.go

pkg/apis/controlplane/v1beta2/sets.go

pkg/controller/networkpolicy/networkpolicy_controller.go

docs/antrea-network-policy.md

lgtm-com · 2022-08-02T01:19:03Z

This pull request introduces 1 alert when merging 8c089ed into 98967d7 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

Address comments and add more validation UT cases. Signed-off-by: wgrayson <[email protected]>

pkg/agent/controller/networkpolicy/reconciler.go

pkg/controller/networkpolicy/networkpolicy_controller.go

Signed-off-by: wgrayson <[email protected]>

GraysonWu · 2022-08-02T04:58:59Z

/test-all

docs/antrea-network-policy.md

tnqn

LGTM

Signed-off-by: wgrayson <[email protected]>

GraysonWu · 2022-08-04T23:00:27Z

/test-all

tnqn

LGTM

tnqn · 2022-08-05T00:57:59Z

@jianjuns do you have more comments on this one?

jianjuns · 2022-08-05T20:26:41Z

@jianjuns do you have more comments on this one?

No.

GraysonWu · 2022-08-05T23:15:22Z

/test-integration

tnqn · 2022-08-08T03:12:26Z

/skip-integration tested manually

GraysonWu requested review from hongliangl, jianjuns and tnqn July 13, 2022 01:32

GraysonWu force-pushed the np-on-svc branch from 15f8736 to b72a300 Compare July 13, 2022 01:51

jianjuns changed the title ~~Add ACNP appliedto Nodeport svc support~~ Add ACNP appliedto NodePort svc support Jul 13, 2022

jianjuns reviewed Jul 13, 2022

View reviewed changes

pkg/controller/types/networkpolicy.go Outdated Show resolved Hide resolved

pkg/apis/controlplane/types.go Outdated Show resolved Hide resolved

pkg/apis/controlplane/v1beta2/types.go Outdated Show resolved Hide resolved

pkg/apis/crd/v1alpha1/types.go Outdated Show resolved Hide resolved

GraysonWu force-pushed the np-on-svc branch 3 times, most recently from a881218 to 0dacb3f Compare July 19, 2022 02:07

GraysonWu force-pushed the np-on-svc branch from 0dacb3f to 0cc328f Compare July 19, 2022 03:19

GraysonWu force-pushed the np-on-svc branch 3 times, most recently from 652d2f6 to 8965521 Compare July 21, 2022 01:21

tnqn reviewed Jul 21, 2022

View reviewed changes

docs/antrea-network-policy.md Outdated Show resolved Hide resolved

docs/antrea-network-policy.md Outdated Show resolved Hide resolved

docs/antrea-network-policy.md Outdated Show resolved Hide resolved

pkg/controller/networkpolicy/clusternetworkpolicy.go Show resolved Hide resolved

tnqn added this to the Antrea v1.8 release milestone Jul 21, 2022

GraysonWu force-pushed the np-on-svc branch 8 times, most recently from 700e60a to 739e7df Compare July 27, 2022 21:57

GraysonWu force-pushed the np-on-svc branch from ee7b9f8 to 8320584 Compare July 28, 2022 23:34

tnqn reviewed Jul 29, 2022

View reviewed changes

pkg/agent/controller/networkpolicy/cache.go Show resolved Hide resolved

pkg/agent/controller/networkpolicy/reject.go Outdated Show resolved Hide resolved

pkg/controller/networkpolicy/networkpolicy_controller.go Outdated Show resolved Hide resolved

tnqn reviewed Aug 1, 2022

View reviewed changes

jianjuns reviewed Aug 1, 2022

View reviewed changes

GraysonWu force-pushed the np-on-svc branch 2 times, most recently from 090b3a6 to 8c089ed Compare August 2, 2022 00:51

Address comments

2bc822d

Address comments and add more validation UT cases. Signed-off-by: wgrayson <[email protected]>

GraysonWu force-pushed the np-on-svc branch from 8c089ed to 2bc822d Compare August 2, 2022 02:34

tnqn reviewed Aug 2, 2022

View reviewed changes

pkg/agent/controller/networkpolicy/reconciler.go Show resolved Hide resolved

pkg/controller/networkpolicy/networkpolicy_controller.go Show resolved Hide resolved

Handle node add/delete event

bbf780c

Signed-off-by: wgrayson <[email protected]>

tnqn added kind/feature Categorizes issue or PR as related to a new feature. action/release-note Indicates a PR that should be included in release notes. labels Aug 2, 2022

GraysonWu requested a review from tnqn August 3, 2022 01:35

tnqn reviewed Aug 4, 2022

View reviewed changes

docs/antrea-network-policy.md Show resolved Hide resolved

tnqn previously approved these changes Aug 4, 2022

View reviewed changes

Add proxyAll restriction in the doc

c4034ad

Signed-off-by: wgrayson <[email protected]>

GraysonWu dismissed tnqn’s stale review via c4034ad August 4, 2022 17:51

tnqn approved these changes Aug 5, 2022

View reviewed changes

tnqn merged commit e48c120 into antrea-io:main Aug 8, 2022

GraysonWu mentioned this pull request Oct 19, 2022

Enhance ACNP Service related feature #4261

Merged

antoninbas mentioned this pull request Nov 1, 2023

Report NetworkPolicy destined for Services as invalid when proxyAll is disabled #5591

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ACNP appliedto NodePort svc support #3997

Add ACNP appliedto NodePort svc support #3997

GraysonWu commented Jul 13, 2022 •

edited by jianjuns

Loading

codecov bot commented Jul 13, 2022 •

edited

Loading

GraysonWu commented Jul 15, 2022

lgtm-com bot commented Jul 19, 2022

lgtm-com bot commented Jul 19, 2022

tnqn commented Jul 21, 2022 •

edited

Loading

GraysonWu commented Jul 21, 2022

tnqn commented Jul 22, 2022

GraysonWu commented Jul 31, 2022

GraysonWu commented Jul 31, 2022

tnqn left a comment

lgtm-com bot commented Aug 2, 2022

GraysonWu commented Aug 2, 2022

tnqn left a comment

GraysonWu commented Aug 4, 2022

tnqn left a comment

tnqn commented Aug 5, 2022

jianjuns commented Aug 5, 2022

GraysonWu commented Aug 5, 2022

tnqn commented Aug 8, 2022

Add ACNP appliedto NodePort svc support #3997

Add ACNP appliedto NodePort svc support #3997

Conversation

GraysonWu commented Jul 13, 2022 • edited by jianjuns Loading

codecov bot commented Jul 13, 2022 • edited Loading

Codecov Report

GraysonWu commented Jul 15, 2022

lgtm-com bot commented Jul 19, 2022

lgtm-com bot commented Jul 19, 2022

tnqn commented Jul 21, 2022 • edited Loading

GraysonWu commented Jul 21, 2022

tnqn commented Jul 22, 2022

GraysonWu commented Jul 31, 2022

GraysonWu commented Jul 31, 2022

tnqn left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Aug 2, 2022

GraysonWu commented Aug 2, 2022

tnqn left a comment

Choose a reason for hiding this comment

GraysonWu commented Aug 4, 2022

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Aug 5, 2022

jianjuns commented Aug 5, 2022

GraysonWu commented Aug 5, 2022

tnqn commented Aug 8, 2022

GraysonWu commented Jul 13, 2022 •

edited by jianjuns

Loading

codecov bot commented Jul 13, 2022 •

edited

Loading

tnqn commented Jul 21, 2022 •

edited

Loading